-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validated newtypes #1454
base: series/0.19
Are you sure you want to change the base?
Validated newtypes #1454
Conversation
Hey @denisrosca , thanks for looking into this. However : validated newtypes should not be rendered by default, for the simple reason that it'd break a lot of user code and that's not really acceptable now that smithy4s has received a reasonable amount of adoption. So it should be an opt-in, controlled by means of smithy metadata (with a key prefixed with |
Working on that 👍 |
@Baccata @kubukoz The main issue is that now the same smithy source file can be rendered in different ways depending if the metadata flag is set to true or false. Consider this smithy definition: namespace newtypes.validated
use smithy4s.meta#unwrap
use alloy#simpleRestJson
@length(min: 1, max: 10)
string ValidatedCity
@length(min: 1, max: 10)
string ValidatedName
@unwrap
@length(min: 1, max: 10)
string ValidatedCountry
structure Person {
@httpLabel
@required
name: ValidatedName
@httpQuery("town")
town: ValidatedCity
@httpQuery("country")
country: ValidatedCountry
}
which gets rendered to: final case class Person(name: ValidatedName, town: Option[ValidatedCity] = None, country: Option[String] = None)
object Person extends ShapeTag.Companion[Person] {
val id: ShapeId = ShapeId("newtypes.validated", "Person")
val hints: Hints = Hints.empty
implicit val schema: Schema[Person] = struct(
ValidatedName.schema.required[Person]("name", _.name).addHints(smithy.api.HttpLabel()),
ValidatedCity.schema.optional[Person]("town", _.town).addHints(smithy.api.HttpQuery.unsafeApply("town")),
ValidatedCountry.underlyingSchema.optional[Person]("country", _.country).addHints(smithy.api.HttpQuery.unsafeApply("country")),
){
Person.apply
}.withId(id).addHints(hints)
} This fails to compile because @length(min: 1)
string httpQuery which when converted from smithy to IR is interpreted as a validated newtype. As a result the hint in
As a workaround I'm excluding the |
I see. This is probably not specific to If the default is "false" (unvalidated newtypes), then you can only go from Shapes from a known previously-generated namespace would be referred to in the style they were generated, and the shapes in the currently-being-generated compilation unit would use the metadata key. Regardless of whether we do it this way or another, I think |
If anything, this is yet another argument for rendering dynamic bindings instead of static ones. As much as I hate to say it, the arguments in favour of dynamic bindings are starting to largely outweigh the ones in favour of static binding.
I disagree (somewhat heavily). I appreciate that validated newtypes may be a desirable feature for users, but it's also not something I want to deal when writing unit tests in smithy4s that involve hints, and arguably the maintainers of smithy4s are the ones who are most expose to the shapes coming from the "smithy.api" namespace ... unless you could macro-ify the validation when dealing with literals. Putting aside the "should we just give up and render dynamic bindings", what we could do is create a meta-trait that would trigger the render of newtypes as validated. The metadata flag would indicate that a model preprocessor should be run to add the meta trait to all relevant shapes. The meta-trait would be stored in the resource of published artifacts, alongside the generated code, and could be inspected by downstream code-gen runs (unlike smithy4s-prefixed metadata, which get eluded). |
We discussed this briefly with Denis, I suggested to try and see how much stuff breaks: if it's more than a handful of locations let's keep them as-is, but if it's just a few I'd say
hmm, so that way users could still apply the meta-trait on a case-by-case basis even if metadata is
Not sure what you mean here - I was thinking of using the |
Well, thing is in order to use the generated metadata, you'd basically have to store a
To be transparent, my concern also has to do with the following aspects :
|
It's already on 0.19 :) |
My bad, haha |
I'm currently exploring this option and have a question about how the I have a My initial attempt was to basically do the same thing we do for What do you think about making this setting a full-blown generator argument instead of passing it via the generated metadata file? |
You make a good point, it'd be silly to load the model twice. However, I'd like to avoid multiplying codegen arguments which need to be forwarded to 3 different "front-ends" (SBT/Mill/cli), in favour of allowing some level of configuration via the reading of a |
@Baccata I had a brief discussion with Denis, we're thinking to "just":
That should resolve the double-loading problem, and the configuration continues to live in the Smithy files. Did we get this right? |
That's sensible, actually ! |
modules/codegen/src/smithy4s/codegen/internals/CodegenImpl.scala
Outdated
Show resolved
Hide resolved
modules/codegen-plugin/src/smithy4s/codegen/Smithy4sCodegenPlugin.scala
Outdated
Show resolved
Hide resolved
Co-authored-by: Jakub Kozłowski <kubukoz@gmail.com>
def build: Validator[A, B] = new ComposedValidator(first, other, bijection) | ||
} | ||
|
||
private final class ComposedValidator[A, B]( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this class and ValidatorBuilder
have exactly the same data structure. There's no reason to separate the Validator
with its Builder
at all then, as you could very well just have a def alsoValidate[C](constraint: C)(implicit ...) : ValidatorBuilder[A, B]
directly on the Validator
interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworked this a bit and got rid of the multiple "builders" and added, as suggested, a new alsoValidate
method.
modules/docs/markdown/04-codegen/01-customisation/13-validated-newtypes.md
Outdated
Show resolved
Hide resolved
|
||
private[internals] final case class CodegenRecord( | ||
namespaces: List[String] | ||
namespaces: List[String], | ||
validatedNewtypes: Option[Boolean] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is intended to be some configuration, this is not the right place for it. That'd be here :
See
case class Config( | |
errorsAsScala3Unions: Boolean, | |
wildcardArgument: String, | |
renderOptics: Boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not meant as "configuration", but more of a record of how namespaces where previously generated.
I'm using this in the ValidatedNewtypesTransformer to make sure that namespaces that were previously generated using this new feature have the proper @validateNewtype
trait applied.
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Yeah I guess that works 👍. The approach I would have preferred would have been to generate an apply @validatedNewType
statement in a bespoke file that would get packaged with the artifact, but I think your approach is sound too, and arguably requires less work.
renderHintsVal(hints), | ||
line"val underlyingSchema: $Schema_[$tpe] = ${tpe.schemaRef}$trailingCalls", | ||
lines( | ||
line"implicit val schema: $Schema_[$name] = $definition$bijection_(underlyingSchema, asBijectionUnsafe)$closing" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're gonna hate me, but the schema of NewtypeValidated
should not be a bijection, it should be a refinement (or a series of refinements)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be more precise : the schema should not call upon asBijectionUnsafe
.
I think the easiest way to achieve this would be to add a def toSchema(a: Schema[A]) : Schema[B]
to the Validator[A, B]
interface, because the ValidatorImpl
keeps everything that you'll need to formulate the correct schema : a list of refinements and a bijection to apply at the end.
|
||
package smithy4s | ||
|
||
abstract class NewtypeValidated[A] extends HasId { self => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ValidatedNewtype
sounds better (as in, it's correct English)
Add an option to render constrained newtypes over Smithy primitives as "validated" newtypes.
Closes #966
PR Checklist (not all items are relevant to all PRs)